Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI bug fix: Kubernetes Role filter replace with explicit input filter #27178

Merged
merged 15 commits into from
Jun 3, 2024

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented May 22, 2024

  • Ent test pass
  • Test coverage

The UI team has run into various issues with the use of NavigateInput for filtering over a list. Such an issue arose in the Kubernetes Secret Engine, and instead of trying to solve for that component, we moved forward with using the new explicit search pattern.

This PR fixes Issue #27153

Note:

  • I created a template only component that plays off the filterInput component, but as the naming suggests is an explicit filter.
  • Whoever consumes this component should handle the actions in the parent. This approach was encouraged because routing and directories, etc can be very individualized per list view (that's part of what makes NavigateInput a hard component to read — it's doing too much individualized tasks).
  • The other areas we use NavigateInput should also move towards this adoption. I'll write a JIRA ticket for this.

Before

Screen.Recording.2024-05-22.at.10.54.53.AM.mov

After

k8after.mov

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label May 22, 2024
@Monkeychip Monkeychip added the ui label May 22, 2024
Copy link

github-actions bot commented May 22, 2024

CI Results:
All Go tests succeeded! ✅

@Monkeychip Monkeychip added this to the 1.17.0 milestone May 22, 2024
@Monkeychip Monkeychip marked this pull request as ready for review May 22, 2024 20:50
@Monkeychip Monkeychip requested a review from a team as a code owner May 22, 2024 20:50
Copy link

github-actions bot commented May 22, 2024

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@andaley andaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great! i had a couple changes requested for tests, let me know if you have any questions about them. aside from that everything else looks good to me. 👍

ui/lib/kubernetes/addon/components/page/roles.js Outdated Show resolved Hide resolved
Comment on lines +54 to +57
// On escape, transition to roles index route.
this.navigate();
}
// ignore all other key events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is slick! nice job including keyboard navigation here. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This originally comes from Chelsea's refactor of an old filter input for Kv (see KvListFilter). All the credit goes to her :)

@Monkeychip Monkeychip requested a review from andaley May 23, 2024 14:52
Copy link
Contributor

@andaley andaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like 2 tests are failing -- probably related to the things i just left comments on. hope that helps!

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some open questions, but very pragmatic approach and helpful description! It might be worth noting that this component is best used in list views where the items do not have a folder/nested structure

@text="Search"
@icon="search"
type="submit"
{{on "click" @handleSearch}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we aren't wrapping these in a <form> element so that clicking this button submits (and then we also get a free submit when the user presses the "Enter" button)? I think that would be a nice ergonomic update to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests 😵‍💫 I originally had it wrapped as a form, but couldn't call the action correctly from the tests. There was an error about it being in a form element. I wish I would have saved that specific error. Let me see if I can resurface it. The enter functionality would be very nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashishaw This was the error, any thoughts? Only on the test, not on the UI.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, we just need to do evt.preventDefault in the triggered action so that it doesn't "send" the form data to the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Found an example in how we do it for sinon spies in toggle-test (thank you—you did that block of code).

@@ -20,6 +20,8 @@ export const GENERAL = {

filter: (name: string) => `[data-test-filter="${name}"]`,
filterInput: '[data-test-filter-input]',
filterInputExplicit: '[data-test-filter-input-explicit]',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@Monkeychip Monkeychip requested a review from hashishaw May 28, 2024 16:06
@Monkeychip
Copy link
Contributor Author

Monkeychip commented May 28, 2024

@hashishaw

it might be worth noting that this component is best used in list views where the items do not have a folder/nested structure

Because this is a template only component, I think the parent who uses it could handle the directory structure search in the actions passed in. My thought was it could be used for any kind of odd routing situation, because it's the parents responsibility to handle the actions. Thoughts?

Copy link
Contributor

@andaley andaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good on my end! 🙌

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.17.x hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants